Skip to content

fix(sqlserver): support verify-full TLS mode - #443

Merged
tianzhou merged 2 commits into
bytebase:mainfrom
tsiakoulias:fix/sqlserver-verify-full-support
Oct 2, 2026
Merged

tianzhou merged 2 commits into
bytebase:mainfrom
tsiakoulias:fix/sqlserver-verify-full-support

Conversation

@tsiakoulias

@tsiakoulias tsiakoulias commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Add sslmode=verify-full support for SQL Server.

  • Enable TLS with certificate and hostname verification.
  • Accept verify-full in TOML configuration.
  • Reject invalid, empty, malformed, and duplicate DSN sslmode values.
  • Keep the current default and valid TLS modes.
  • Update the configuration documentation.

Tests

Add unit tests for:

  • The verify-full driver options and TOML configuration.
  • Invalid and duplicate DSN modes.
  • Encoded modes and credentials.
  • TOML mode conflicts and DSN processing.

Additionally, a separate fix for empty TOML sslmode values is proposed in #451.

Enable certificate and hostname verification when sslmode=verify-full is explicit. Accept the mode in TOML, document Node trust configuration, and add focused parser and configuration tests. Preserve existing defaults and parsing behavior.
Copilot AI balanced review requested due to automatic review settings September 29, 2026 01:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@tianzhou

Copy link
Copy Markdown
Member

Please open an issue first

@tsiakoulias

Copy link
Copy Markdown
Contributor Author

Thanks. I opened #446 to describe the request.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unsupported SQL Server TLS modes can silently fall back to plaintext instead of failing closed.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread src/connectors/sqlserver/index.ts
@tsiakoulias

Copy link
Copy Markdown
Contributor Author

Fixed copilot findings. The SQL Server parser now rejects unsupported, empty, malformed, and duplicate sslmode values before connection setup. Added parser and TOML regression tests. Existing defaults and valid modes remain unchanged.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation consistently maps verified TLS settings, validates inputs, and includes focused unit coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@tsiakoulias

Copy link
Copy Markdown
Contributor Author

While checking this PR, I found that the shared TOML loader can replace an empty sslmode with the DSN value before validation. This also affects other database engines, so I addressed it separately in #451.

@tianzhou tianzhou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the contribution.

@tianzhou
tianzhou merged commit 4ddb26e into bytebase:main Oct 2, 2026
3 checks passed
@tsiakoulias
tsiakoulias deleted the fix/sqlserver-verify-full-support branch October 2, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants